fix(install/preflight): cos #103
Conversation
longhorn/longhorn-9664 Signed-off-by: Chin-Ya Huang <[email protected]>
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/remote/preflight/installer.go (1)
Line range hint
134-234: Consider refactoring the shell script for improved maintainability and robustnessWhile the main issue has been resolved, there are a few suggestions to improve the script:
Consider breaking down the large shell script into smaller, more manageable functions or even separate files. This would improve readability and maintainability.
Add error handling for the
catcommand in thecheck_operating_systemfunction. For example:function check_operating_system() { if [ ! -f "${HOST_MOUNT_DIR}/etc/os-release" ]; then echo "ERROR: ${HOST_MOUNT_DIR}/etc/os-release file not found" exit 1 fi os=$(cat "${HOST_MOUNT_DIR}/etc/os-release" | grep '^ID=' | cut -d= -f2) if [ "$os" != "cos" ]; then echo "ERROR: Operating system ($os) is not Container Optimized OS (cos)" exit 1 fi }
- Consider using
/bin/shinstead of/bin/bashfor better compatibility across different systems, unless you specifically need Bash features.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/remote/preflight/installer.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
pkg/remote/preflight/installer.go (2)
147-147: Correct OS detection for Container Optimized OSThis change addresses the issue described in the PR objectives by modifying the path used to detect the operating system. By using
${HOST_MOUNT_DIR}/etc/os-releaseinstead of/etc/os-release, the script now correctly reads the OS information from the host system rather than from within the container.This fix should resolve the bug where the OS was incorrectly identified as "sles" instead of "cos" (Container Optimized OS).
Line range hint
1-605: Overall impact of the change is positive and well-integratedThe modification to the OS detection logic in the
newConfigMapForContainerOptimizedOSfunction is well-integrated with the rest of the code. TheInstallByContainerOptimizedOSandnewDaemonSetForContainerOptimizedOSfunctions correctly utilize the updated ConfigMap, ensuring that the fix for OS detection is properly applied throughout the installation process.This change should resolve the issue of incorrect OS detection on Container Optimized OS without introducing any apparent side effects or requiring changes to other parts of the codebase.
|
Test result: |
|
@mergify backport v1.7.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9664
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes